Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the bug where raw is abead of obj-temp #175

Conversation

yiraeChristineKim
Copy link
Contributor

@yiraeChristineKim yiraeChristineKim commented Aug 7, 2024

There is a bug when raw-template manifest is ahead of obj-template manifest.
https://issues.redhat.com/browse/ACM-12563

@mprahl
Copy link
Member

mprahl commented Aug 7, 2024

@yiraeChristineKim I think this is getting too complicated. How about this alternative implementation?

diff --git a/internal/utils.go b/internal/utils.go
index 39b2cd9..92414a5 100644
--- a/internal/utils.go
+++ b/internal/utils.go
@@ -169,11 +169,9 @@ func getPolicyTemplates(policyConf *types.PolicyConfig) ([]map[string]interface{
        objectTemplates := make([]map[string]interface{}, 0, objectTemplatesLength)
        policyTemplates := make([]map[string]interface{}, 0, policyTemplatesLength)
 
-       policyNameCounter := 0
-       // This is used to identify the first `manifests[].name` when `ConsolidateManifests` is set to true.
-       // If `manifests[].name` is not present, the consolidated Configuration policy will
-       // default to using `policies.name`.
-       firstObjTempNameIndex := -1
+       var consolidatedPolicyName string
+
+       policyNameCounter := map[string]int{}
 
        for i, manifestGroup := range manifestGroups {
                complianceType := policyConf.Manifests[i].ComplianceType
@@ -183,7 +181,10 @@ func getPolicyTemplates(policyConf *types.PolicyConfig) ([]map[string]interface{
                ignorePending := policyConf.Manifests[i].IgnorePending
                extraDeps := policyConf.Manifests[i].ExtraDependencies
 
-               manifestNameCounter := 0
+               policyName := policyConf.Manifests[i].Name
+               if policyName == "" {
+                       policyName = policyConf.Name
+               }
 
                for _, manifest := range manifestGroup {
                        err := setGatekeeperEnforcementAction(manifest,
@@ -208,11 +209,13 @@ func getPolicyTemplates(policyConf *types.PolicyConfig) ([]map[string]interface{
 
                                _, found, _ := unstructured.NestedString(manifest, "object-templates-raw")
                                if found {
+                                       policyNameCounter[policyName]++
+
                                        policyTemplate = buildPolicyTemplate(
                                                policyConf,
                                                manifest["object-templates-raw"],
                                                &policyConf.Manifests[i].ConfigurationPolicyOptions,
-                                               getConfigurationPolicyName(policyConf, i, &policyNameCounter, &manifestNameCounter),
+                                               getConfigurationPolicyName(policyName, policyNameCounter[policyName]),
                                        )
                                } else {
                                        policyTemplate = map[string]interface{}{"objectDefinition": manifest}
@@ -260,28 +263,27 @@ func getPolicyTemplates(policyConf *types.PolicyConfig) ([]map[string]interface{
                        }
 
                        if policyConf.ConsolidateManifests {
+                               if consolidatedPolicyName == "" {
+                                       consolidatedPolicyName = policyConf.Manifests[i].Name
+                               }
                                // put all objTemplate with manifest into single consolidated objectTemplates
                                objectTemplates = append(objectTemplates, objTemplate)
                        } else {
                                // casting each objTemplate with manifest to objectTemplates type
                                // build policyTemplate for each objectTemplates
+                               policyNameCounter[policyName]++
+
                                policyTemplate := buildPolicyTemplate(
                                        policyConf,
                                        []map[string]interface{}{objTemplate},
                                        &policyConf.Manifests[i].ConfigurationPolicyOptions,
-                                       getConfigurationPolicyName(policyConf, i,
-                                               &policyNameCounter, &manifestNameCounter),
+                                       getConfigurationPolicyName(policyName, policyNameCounter[policyName]),
                                )
 
                                setTemplateOptions(policyTemplate, ignorePending, extraDeps)
 
                                policyTemplates = append(policyTemplates, policyTemplate)
                        }
-
-                       // If `firstObjTempNameIndex` is already set, skip this step.
-                       if firstObjTempNameIndex == -1 && policyConf.Manifests[i].Name != "" {
-                               firstObjTempNameIndex = i
-                       }
                }
        }
 
@@ -294,13 +296,19 @@ func getPolicyTemplates(policyConf *types.PolicyConfig) ([]map[string]interface{
        // just build one policyTemplate by using the above non-empty consolidated objectTemplates
        // ConsolidateManifests = true or there is non-policy-type manifest
        if policyConf.ConsolidateManifests && len(objectTemplates) > 0 {
+               if consolidatedPolicyName == "" {
+                       consolidatedPolicyName = policyConf.Name
+               }
+
+               policyNameCounter[consolidatedPolicyName]++
+
                // If ConsolidateManifests is true and multiple manifest[].names are provided, the configuration policy
                // name will be the first name of manifest[].names
                policyTemplate := buildPolicyTemplate(
                        policyConf,
                        objectTemplates,
                        &policyConf.ConfigurationPolicyOptions,
-                       getConfigurationPolicyName(policyConf, firstObjTempNameIndex, &policyNameCounter, nil),
+                       getConfigurationPolicyName(consolidatedPolicyName, policyNameCounter[consolidatedPolicyName]),
                )
                setTemplateOptions(policyTemplate, policyConf.IgnorePending, policyConf.ExtraDependencies)
                policyTemplates = append(policyTemplates, policyTemplate)
@@ -336,42 +344,12 @@ func getPolicyTemplates(policyConf *types.PolicyConfig) ([]map[string]interface{
        return policyTemplates, nil
 }
 
-// getConfigurationPolicyName returns the `ConfigurationPolicy` name based on the provided PolicyConfig.
-// When a name is assigned, it increments the associated counter, using it as a suffix if it's greater
-// than one to create unique names. If both `manifest[].name` and `policies.name` are provided,
-// `manifest[].name` takes priority as the configuration name.
-func getConfigurationPolicyName(policyConf *types.PolicyConfig, manifestGroupIndex int,
-       policyNameCounter *int, manifestNameCounter *int,
-) string {
-       var configName string
-
-       if manifestGroupIndex >= 0 {
-               // Do not append a number to configName when it is used for the first time.
-               configName = policyConf.Manifests[manifestGroupIndex].Name
-
-               // For the case where ConsolidateManifests is true and the manifest's name is provided,
-               if manifestNameCounter == nil && configName != "" {
-                       return configName
-               }
-
-               if manifestNameCounter != nil && configName != "" {
-                       *manifestNameCounter++
-                       if *manifestNameCounter > 1 {
-                               return fmt.Sprintf("%s%d", configName, *manifestNameCounter)
-                       }
-
-                       return configName
-               }
-       }
-
-       configName = policyConf.Name
-
-       *policyNameCounter++
-       if *policyNameCounter > 1 {
-               return fmt.Sprintf("%s%d", configName, *policyNameCounter)
+func getConfigurationPolicyName(name string, count int) string {
+       if count > 1 {
+               return fmt.Sprintf("%s%d", name, count)
        }
 
-       return configName
+       return name
 }
 
 // setGatekeeperEnforcementAction function override gatekeeper.constraint.enforcementAction

@yiraeChristineKim yiraeChristineKim changed the title Fix the bug where obj-temp is abead of raw Fix the bug where raw is abead of obj-temp Aug 7, 2024
@yiraeChristineKim
Copy link
Contributor Author

@mprahl map[string]int{} This is idea is amazing! Alien idea

When raw-template manifest is ahead of obj-temp manifest, the numbering of policyName doesn't work properly
Signed-off-by: yiraeChristineKim <[email protected]>
Copy link

openshift-ci bot commented Aug 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mprahl, yiraeChristineKim

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [mprahl,yiraeChristineKim]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 243dd4a into open-cluster-management-io:main Aug 9, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants